-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REPLCompletions: async cache PATH scan #52833
REPLCompletions: async cache PATH scan #52833
Conversation
8b91117
to
50e57fb
Compare
Some numbers Natively on an M2 Mac
with On a Windows VM via Parallels on the same M2 Mac
with Clearly it's a good thing to do based on these Windows numbers, and I think @topolarity has durations in the many seconds on WSL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much don't like this happening (even in the background) just because REPL happens to be loaded. But I like that it would be cached. Probably should just be cached per-statement though? Seems very awkward if rm("foo")
and touch("foo")
don't work correctly
Moving to per statement sounds good. I think we do want to do it in the background still, just because of the pathological case @topolarity has on WSL where the PATH scan takes > 5s
Note that this is for files reached by PATH only. Files in the current dir, or a specific path aren't cached, so your examples won't change behavior. |
Yes, the change to move to the background (when first needed by that statement) is very good too. We should try not to run arbitrary computations in the REPL/UI task which more properly belong to a worker task (until the user hits tab). |
Ok. I've moved it to cache in a worker thread at the first moment it would be needed, which is likely to be a hint when typing initially in Side note. I noticed we don't provide these PATH completions for command strings. That seems logical to add. |
Not waiting does seem optimal to me. Lag is generally bad. Can we hook into the |
Side side note: we use completely different code for completions inside |
Sounds good. I think that's what #52847 is trying to achieve |
Come to think of it, if #52847 is done, then |
Yeah, crawling PATH is quite slow on my WSL2 instance since WSL automatically appends the Windows PATH to the Linux one: ┌ Debug: caching PATH files took 25.125992393 seconds
│ length(pathdirs) = 36
│ length(PATH_cache) = 7730
└ @ REPL.REPLCompletions ~/repos/julia/usr/share/julia/stdlib/v1.11/REPL/src/REPLCompletions.jl:338 Even so, with the latest changes the shell feels immediately responsive 👍 The Julia start-up time also looks comparable to before this PR, and it still seems to have useful auto-complete for many commands while it continues scanning. Overall, it feels much better than before. Thanks! |
0.01s vs 25s... YMMV... Good to know we can handle that case nicely. I'll go ahead and merge when CI is green |
# Look for files in PATH as well. These are cached in `cache_PATH` in a separate task in REPL init. | ||
# If we cannot get lock because its still caching just pass over this so that initial | ||
# typing isn't laggy. If the PATH string has changed since last cache re-cache it | ||
cached_PATH_changed() && Base.errormonitor(Threads.@spawn REPLCompletions.cache_PATH()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also rescan every command? How do we make sure this gets periodically refreshed so when the user changes the filesystem (e.g. installs a missing package), the completions aren't wrong afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Where would you call this? I'm a little lost in where to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we spawn it every time it's needed, unless its already running?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it could be interesting to spawn an update almost continually if the past one is finished
Fixes #52765
In
shell>
when first typing a word tab completion hinting searches the entire PATH list for completions again and again for every char entry.On slower systems the hinting can make typing quite slow because of this i.e. #52765
This treats the contents reachable by PATH as static for a given session unless the PATH string changes, by caching it once at startup async and then if PATH changes at tab-completion time.
A modification would be to instead do something like update the cache on an async timer.